-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix HMR in MDX deps in Content Collections #9956
Conversation
🦋 Changeset detectedLatest commit: 2709497 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
!preview cc-hmr |
Snapshots have been released for the following packages:
Publish Log
Build Log
|
This fixes the issue for me. Copying the comment I made in the mentioned issue:
|
Just tested on the few Starlight related scenarios I had in my notes having that issue, and just like Hippo, it now works perfectly with the preview release 👍 |
Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s wild that this works, but I’m really glad it does! Nice one.
const collectedStyles = ${stringifiedStyles}; | ||
const collectedScripts = ${stringifiedScripts}; | ||
|
||
function getPropagatedAssets() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dang. Clever solution! Not too clever of course, just the right amount of clever. 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two small comments, thank you very much for taking this one! Gonna be a big productivity boost, especially for Starlight users.
@@ -0,0 +1,29 @@ | |||
|
|||
// TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there something to be done here?
@@ -62,6 +62,9 @@ class VirtualVolume extends Volume { | |||
class VirtualVolumeWithFallback extends VirtualVolume { | |||
// Fallback to the real fs | |||
readFile(p, ...args) { | |||
if(p?.toString().includes('astro:dev-module-loader')) { | |||
throw new Error('This module can\'t be loaded threw here. blah'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how likely that error is, but if it's real, a good error message would be better so we don't eventually have to explain to a new contributor why that error happens, ha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this could break markdoc?
astro/packages/integrations/markdoc/components/TreeNode.ts
Lines 126 to 128 in 6d273be
} else if (isPropagatedAssetsModule(node.name)) { | |
const { collectedStyles, collectedLinks, collectedScripts } = node.name; | |
const component = (await node.name.getMod()).default; |
I'm not very comfortable with the .ssrModule
assignment because it means crossing an explicitly separate boundary between the "Vite-plugin runtime" and "SSR app runtime". It can also make supporting other envs in the future, like workerd in dev, harder.
(Collapsing my comments because it's outdated)
It looks like the core issue here is that if /my/component/Foo.astro
is invalidated, we should also invalidate /my/component/Foo.astro?astroPropagatedAssets
hardly. I can't think of a cleaner fix today, but if we need to workaround it, maybe we should directly fix the invalidation, e.g. patching server.moduleGraph.invalidateModule
, check if we're invalidating an .astro
module, then try to invalidate the .astro?astroPropagatedAssets
module as well?
I think what Vite can fix, which I can try to implement, is that the /my/component/Foo.astro?astroPropagatedAssets
transform can call this.addWatchFile('/my/component/Foo.astro')
, which implies that even if we import it in code, whenever the watched file is changed, we should still hard-invalidate it.
EDIT: I got a better idea of the problem now today. The issue is that when any imported modules recursively down from the propagated module is updated, the propagated module also needs to be invalidated. Because getStylesForURL
and getScriptsForURL
crawls all of its imported modules recursively to retrieve that.
The proper way is to register all of its imported modules with this.addWatchFile
, maybe get...ForURL
can return that based on the walked mod.file
. Another workaround is to simply invalidate all propagated modules whenever handleHotUpdate
is invoked, so that they are re-transformed on the next request. Or another workaround is this PR, so I could see this PR being something worth considering too 🤔 I think I'd still lean on the first way (in this paragraph) though.
!preview cc-hmr |
Snapshots have been released for the following packages:
Publish Log
Build Log
|
lgtm, willing to try this approach. |
Changes
entry.render()
is called in dev, it gets fresh set of styles/scripts, so it should always be right.Testing
Docs
N/A, bug fix